Skip to content

Conversation

@kdaviduik
Copy link
Contributor

@kdaviduik kdaviduik commented Feb 10, 2026

WHY are these changes introduced?

Closes https://github.com/Shopify/developer-tools-team/issues/1034.

Cart AJAX endpoints (/cart/add.js, /cart/update.js, /cart/change.js, /cart.js, /cart.json) return 401 Unauthorized when running shopify theme dev on CLI 3.89.0, but work correctly on 3.88.0. 5+ developers affected with confirmed reproduction.

WHAT is this pull request doing?

Conditionally omits the Authorization: Bearer <storefrontToken> header on proxied requests to endpoints that use session-cookie auth (cart, checkout, account). CDN/asset paths continue receiving the Bearer token for theme preview rendering.

Changes:

File Change
proxy.ts Added CART_SESSION_PATTERN constant, needsBearerToken() helper, updated Authorization conditional
proxy.test.ts Added 15 regression tests for Bearer token header behavior
.changeset/fix-cart-ajax-401.md Patch bump for @shopify/theme

Key design decisions:

  1. Denylist over allowlist: The set of cookie-auth endpoints is small and stable (cart, checkout, account). The set of Bearer-needing endpoints is large and open-ended (CDN assets, vanity CDN rewrites, .js.liquid files, etc.). An allowlist would risk silent rendering failures; a denylist's failure mode is a loud 401 — easy to detect.

  2. Separate CART_SESSION_PATTERN from CART_PATTERN: CART_PATTERN (/^\/cart\//) is for routing — it deliberately excludes GET /cart (the cart HTML page, which should render locally). CART_SESSION_PATTERN (/^\/cart(\/|\.js|\.json|$)/) is for auth — broader because ALL cart paths use session-cookie auth. Collapsing these would contaminate routing with auth concerns.

  3. Reuse CHECKOUT_PATTERN and ACCOUNT_PATTERN: Their routing and auth scopes currently align. The comment in needsBearerToken() warns future editors that changes to those patterns should consider auth implications.

  4. Query string stripping: needsBearerToken() strips query strings before regex matching to ensure ACCOUNT_PATTERN's $ anchor matches correctly (e.g., /account?foo=bar).

How to test your changes?

Automated:

npx vitest run packages/theme/src/cli/utilities/theme-environment/proxy.test.ts
npx vitest run packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts

Manual (end-to-end):
Note: I'm currently having 1Password issues so I'm locked out of the admin and haven't been able to manually test these myself

  1. Run shopify theme dev on a test store
  2. Navigate to a product page
  3. Execute:
    curl -X POST "http://127.0.0.1:9292/cart/add.js" \
      -H "Content-Type: application/json" \
      -d '{"items":[{"id":VARIANT_ID,"quantity":1}]}'
  4. Confirm response is 200 OK (not 401)
  5. Verify theme assets still load correctly (CSS, JS) — confirms Bearer token still sent for CDN/asset paths
  6. Verify CDN assets still load correctly

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

Cart AJAX endpoints (/cart/add.js, /cart/update.js, /cart/change.js,
/cart.js, /cart.json) return 401 when running `shopify theme dev` on
CLI 3.89.0, but work correctly on 3.88.0. 5+ developers affected.

Root cause: Commit dda10ad added `Authorization: Bearer
<storefrontToken>` to all proxied requests. Cart endpoints authenticate
via session cookies (_shopify_essential). When SFR's
BearerTokenAuthorizationMiddleware sees the Authorization header, it
selects token-based auth instead of cookie auth. The CLI's Storefront
API token lacks cart scopes (write_global_api_universal_cart), so
GlobalApi::Authorize.perform() fails with 401.

Fix: Added needsBearerToken() helper that returns false for cart,
checkout, and account paths (session-cookie auth), and true for
CDN/asset paths (need Bearer token for theme preview rendering).

Key decisions:
- Denylist (exclude specific paths) over allowlist because the set of
  cookie-auth endpoints is small/stable while Bearer-needing paths are
  open-ended. An allowlist would risk silent rendering breakage.
- Separate CART_SESSION_PATTERN from CART_PATTERN because routing and
  auth have different scopes (routing excludes GET /cart to render
  locally; auth excludes all cart paths from Bearer tokens).
- Reuses CHECKOUT_PATTERN and ACCOUNT_PATTERN from routing since their
  routing and auth scopes currently align.
@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 2026

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 79.69% 14465/18151
🟡 Branches 74.06% 7185/9701
🟡 Functions 79.81% 3689/4622
🟢 Lines 80.05% 13673/17081

Test suite run success

3733 tests passing in 1438 suites.

Report generated by 🧪jest coverage report action from ac00b75

Flatten describe nesting to comply with vitest/max-nested-describe (max: 2)
and use index signature style per consistent-indexed-object-style rule.
@kdaviduik kdaviduik marked this pull request as ready for review February 10, 2026 07:35
@kdaviduik kdaviduik requested review from a team as code owners February 10, 2026 07:35
Copy link
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR, @kdaviduik! It looks good to me and works as expected as well:

Before:
Image

After:
Image

Great stuff!

@karreiro
Copy link
Contributor

👋 Hey @Shopify/app-inner-loop, could you please take a look at this when you have a moment?

PS: I've proposed this PR to prevent similar review requests in the future 🙇

@kdaviduik kdaviduik added this pull request to the merge queue Feb 10, 2026
Merged via the queue into main with commit 962d1f5 Feb 10, 2026
26 checks passed
@kdaviduik kdaviduik deleted the kd-add-to-cart-bug branch February 10, 2026 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants